Skip to content

nixos-option: move shellcheck validation to passthru.tests.shellcheck#372722

Merged
Mic92 merged 1 commit intoNixOS:masterfrom
misuzu:nixos-option-riscv64-eval-fix
Jan 11, 2025
Merged

nixos-option: move shellcheck validation to passthru.tests.shellcheck#372722
Mic92 merged 1 commit intoNixOS:masterfrom
misuzu:nixos-option-riscv64-eval-fix

Conversation

@misuzu
Copy link
Copy Markdown
Contributor

@misuzu misuzu commented Jan 10, 2025 •
edited
Loading

Fixes eval for riscv64-linux, see #369151 (comment)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@misuzu misuzu mentioned this pull request Jan 10, 2025
Merged
13 tasks
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 10, 2025
@misuzu misuzu force-pushed the nixos-option-riscv64-eval-fix branch from 7bcd2c3 to 0bf4ce2 Compare January 10, 2025 18:54
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 10, 2025
@ElvishJerricco
Copy link
Copy Markdown
Contributor

ElvishJerricco commented Jan 10, 2025

huh, almost feels like there should be a shellcheck hook or something that does this logic automatically. Oh well; probably not worth bothering over.

lf-
lf- previously requested changes Jan 10, 2025
View reviewed changes
pkgs/by-name/ni/nixos-option/package.nix Outdated
Copy link
Copy Markdown
Member

@lf- lf- Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tryEval is evil, can we consider not doing this if possible? e.g. ensuring that the ghc/shellcheck is marked unavailable as appropriate?

Copy link
Copy Markdown
Contributor Author

@misuzu misuzu Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is the only way it works, see #231537 (comment)

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 10, 2025

@misuzu it would be useful if we would not need ghc as a build dependency in general to build nixos-option.
Could you just move the check to passthru.tests instead? Than we also don't need the complex testing logic.

@misuzu misuzu force-pushed the nixos-option-riscv64-eval-fix branch from 0bf4ce2 to 7f3a5d0 Compare January 11, 2025 10:43
@misuzu misuzu changed the title nixos-option: fix evaluation for riscv64-linux nixos-option: move shellcheck validation to passthru.tests.shellcheck Jan 11, 2025
@misuzu
Copy link
Copy Markdown
Contributor Author

misuzu commented Jan 11, 2025

Could you just move the check to passthru.tests instead? Than we also don't need the complex testing logic.

Done

@misuzu misuzu requested a review from lf- January 11, 2025 10:45
@Mic92 Mic92 dismissed lf-’s stale review January 11, 2025 10:46

That part is gone.

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 11, 2025
@nix-owners nix-owners bot requested review from Aleksanaa, FireyFly and azuwis January 11, 2025 10:51
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 11, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372722

x86_64-linux

✅ 2 packages built:
  • home-manager
  • nixos-option

@Mic92 Mic92 merged commit d090642 into NixOS:master Jan 11, 2025
@misuzu misuzu deleted the nixos-option-riscv64-eval-fix branch January 11, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thiagokokada thiagokokada thiagokokada approved these changes

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@lf- lf- Awaiting requested review from lf-

@FireyFly FireyFly Awaiting requested review from FireyFly

@azuwis azuwis Awaiting requested review from azuwis

@Aleksanaa Aleksanaa Awaiting requested review from Aleksanaa

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@misuzu @ElvishJerricco @Mic92 @thiagokokada @lf- @wegank